Skip to content

Conversation

@ankuns
Copy link
Contributor

@ankuns ankuns commented Apr 16, 2025

The SoC output power starts to rise earlier than at the EVENTS_READY of the RADIO. To compensate this phenomenon and avoid spurious emission issue additional Kconfig option for the nRF21540 FEM MPSL_FEM_NRF21540_PA_LEAD_TIME_ADDITIONAL_US is added.

The default value is selected so that the TX_EN pin of the nrf21540 is enabled 26us before EVENTS_READY.

In the NCS 3.0 release the same spurious emission issue has been fixed by simply setting default value of tx-en-settle-time-us to 26. https://github.com/nrfconnect/sdk-zephyr/blob/v4.0.99-ncs1-branch/dts/bindings/net/wireless/nordic%2Cnrf21540-fem.yaml#L98 . However, to provide the fix without [nrf noup] commit (unwanted, see here ) the additional parameter on the nRF Connect SDK level is proposed.

Ref: KRKNWK-17752

The SoC output power starts to rise earlier than at the EVENTS_READY
of the RADIO. To compensate this phenomenon and avoid spurious emission
issue additional Kconfig option for the nRF21540 FEM
MPSL_FEM_NRF21540_PA_LEAD_TIME_ADDITIONAL_US is added.

The default value is selected so that the TX_EN pin of the nrf21540
is enabled 26us before EVENTS_READY.

Ref: KRKNWK-17752

Signed-off-by: Andrzej Kuros <[email protected]>
@github-actions github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Apr 16, 2025
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Apr 16, 2025

CI Information

To view the history of this post, clich the 'edited' button above
Build number: 4

Inputs:

Sources:

sdk-nrf: PR head: 341b4af43d5c187a299f63b898a28f215bb852b7

more details

sdk-nrf:

PR head: 341b4af43d5c187a299f63b898a28f215bb852b7
merge base: d026b0142a95a72ad68c6ca5282ff037a7e8a3e7
target head (main): f37bacaf63239faf6afb0bd592bcfda26105c37a
Diff

Github labels

Enabled Name Description
ci-disabled Disable the ci execution
ci-all-test Run all of ci, no test spec filtering will be done
ci-force-downstream Force execution of downstream even if twister fails
ci-run-twister Force run twister
ci-run-zephyr-twister Force run zephyr twister
List of changed files detected by CI (5)
doc
│  ├── nrf
│  │  ├── app_dev
│  │  │  ├── device_guides
│  │  │  │  ├── fem
│  │  │  │  │  ├── fem_nRF21540_optional_properties.rst
│  │  │  │  │  │ fem_nrf21540_gpio.rst
subsys
│  ├── mpsl
│  │  ├── fem
│  │  │  ├── Kconfig
│  │  │  ├── nrf21540_gpio
│  │  │  │  │ mpsl_fem_nrf21540_gpio.c
│  │  │  ├── nrf21540_gpio_spi
│  │  │  │  │ mpsl_fem_nrf21540_gpio_spi.c

Outputs:

Toolchain

Version: 7cbc0036f4
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:7cbc0036f4_8bf7ca4353

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

  • ◻️ Toolchain - Skipped: existing toolchain is used
  • ✅ Build twister
    • sdk-nrf test count: 900
  • ✅ Integration tests
    • ✅ test_ble_nrf_config
    • ✅ test-fw-nrfconnect-ble_samples
    • ✅ test-fw-nrfconnect-chip
    • ✅ test-fw-nrfconnect-rs
    • ✅ test-fw-nrfconnect-fem
    • ✅ test-fw-nrfconnect-thread
    • ✅ test-sdk-find-my
Disabled integration tests
    • desktop52_verification
    • doc-internal
    • test-fw-nrfconnect-apps
    • test-fw-nrfconnect-ble_mesh
    • test-fw-nrfconnect-boot
    • test-fw-nrfconnect-nfc
    • test-fw-nrfconnect-nrf-iot_cloud
    • test-fw-nrfconnect-nrf-iot_libmodem-nrf
    • test-fw-nrfconnect-nrf-iot_mosh
    • test-fw-nrfconnect-nrf-iot_positioning
    • test-fw-nrfconnect-nrf-iot_samples
    • test-fw-nrfconnect-nrf-iot_serial_lte_modem
    • test-fw-nrfconnect-nrf-iot_thingy91
    • test-fw-nrfconnect-nrf-iot_zephyr_lwm2m
    • test-fw-nrfconnect-nrf_crypto
    • test-fw-nrfconnect-proprietary_esb
    • test-fw-nrfconnect-ps
    • test-fw-nrfconnect-rpc
    • test-fw-nrfconnect-tfm
    • test-low-level
    • test-sdk-audio
    • test-sdk-dfu
    • test-sdk-mcuboot
    • test-sdk-pmic-samples
    • test-sdk-wifi
    • test-secdom-samples-public

Note: This message is automatically posted and updated by the CI

@ankuns ankuns requested review from e-rk and piotrkoziar April 16, 2025 06:52
The MPSL_FEM_NRF21540_PA_LEAD_TIME_ADDITIONAL_US Kconfig option is
added. This commit adjusts doc on where it is used.

Signed-off-by: Andrzej Kuros <[email protected]>
@github-actions github-actions bot added the doc-required PR must not be merged without tech writer approval. label Apr 16, 2025
@ankuns ankuns marked this pull request as ready for review April 16, 2025 08:24
@ankuns ankuns requested review from a team as code owners April 16, 2025 08:24
Copy link
Contributor

@nordicjm nordicjm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understandable that you need a different value for softdevice, but where has the thought been to how customers will use this? This is a devicetree property, it is configured in devicetree, now suddenly it's configured in devicetree and Kconfig - how does that make sense? @carlescufi

@ankuns
Copy link
Contributor Author

ankuns commented Apr 30, 2025

@nordicjm , regarding #21842 (review) .
Tagging also @carlescufi

The ncs-radio-sw is code owner of the FEM in sdk-nrf. Otherwise what's the point of ownership. We prefer this way of fix. Possibly everything can be do better, but we have limited time/human and resources. Our goal is to fix customers' issue, and have it fixed by default, so that the customers don't need to tweak these values on their own while providing them ability to do so if they need. We believe that customers will use NCS "as is". They will tweak parameters if and only if they have to.

Approaches considered so far:

  1. Set tx-en-settle-time-us in sdk-zephyr to 26, with [nrf noup] commit (have it already on the ncs-v3.0-branch) As discussed [nrf noup] dts: nrf21540-fem default tx-en-settle-time-us=26 sdk-zephyr#2762 (comment), you don't like the noup approach.
  2. Set the tx-en-settle-time-us in upstream zephyr to 26 will not find any justification in upstream as discussed in the mentioned discussion.
  3. Set the tx-en-settle-time-us for each board to 26 will be overkill, IMHO it violates heavily the DRY rule.
  4. Write in doc to use 26.... Discarded, why to elaborate on how to fix instead of providing the fix.
  5. This PR has none of above flaws. So no change in upstream Zephyr that's hard to justify. No nrf noup commit, that you consider a technical debt and hard to maintain.

Reminder on the "why is this even needed": The need to increase the lead time when TX_EN pin is activated comes from the way the SoC is constructed AND the way the software in NCS (not in upstream Zephyr) handles it. The default value of 11 for tx-en-settle-time-us is a property of nrf21540 device. The value 11 is perfect, when it comes to just the meaning of the parameter alone and it's description. The additional lead time is a result of the fact that RF power starts to rise earlier than on RADIO.READY and because the protocol drivers express RADIO.READY as the target point in time when FEM should be ready. This additional time is rather a property of a SoC not the FEM. Correct operation of the nRF21540 on given SoC requires taking into account both values.

The alternatives:
a. Move the nordic,nrf21540*.yaml files from https://github.com/zephyrproject-rtos/zephyr/tree/main/dts/bindings/net/wireless to sdk-nrf. In NCS the value could be tx-en-settle-time-us 26 and justified in the description.
b. Turn the proposed Kconfig into a macro, internal to the subsys/mpsl/fem/nrf21540_gpio/mpsl_fem_nrf21540_gpio.c module. User's will have no ability to change it without changing the code. Kconfig is more handy.
c. Modify the MPSL binaries for the nrf21540 so that the additional time 15us is added anyway, but the user cannot control it in any way. We prefer configurable behavior and reasonable defaults over fixed behavior that can't be modified.
d. Fix all protocol drivers (any volunteer?) so that when they setup FEM for tx they set the target point to be the moment when RF starts to rise instead of current event READY. This is very labor intensive, with no more benefit that the current proposal provides. By the way, other FEMs for which the issue has already been addressed in different way would need also an update, what currently is out of scope.
e. Add a new DT property for the nordic,nrf-radio.yaml (in upstream Zephyr + cherry pick to downstream) expressing the time between RF power starts to rise and ready event. It could be different for each SoC type, it would replace the proposed MPSL_FEM_NRF21540_PA_LEAD_TIME_ADDITIONAL_US Kconfig.

Please pick one of proposed alternative, propose better one or approve this PR as it is. We have limited time.

@nordicjm
Copy link
Contributor

Fix it in boards that need it, this is literally what a board dts file is for

@ankuns
Copy link
Contributor Author

ankuns commented Apr 30, 2025

Please pick one of proposed alternative, propose better one or approve this PR as it is. We have limited time.

What I asked for was "Please pick one of proposed alternative, propose better one or approve this PR as it is."

What you are proposing is one of the discarded solutions. It is worse because:

  • it requires fixing in many places within board dts files providing possibly a copy-pasted comment explaining why it is necessary, whereas my proposal follows the DRY principle.
  • some of boards are already in upstream, where changing from 11 to 26 is unjustified without referencing to downstream nRF Connect SDK. Those boards (nrf21540dk, thingy53) in upstream zephyr are in fact incomplete as the upstream zephyr does not provide any fem drivers. My proposal does not require to modify upstream in any way, but fixes also issue on boards in upstream without modifying them.
  • customers will need to fix their boards on their own, whereas my proposal fixes also their boards.

Please pick different one, from "the alternatives" that I enumerated.

@ankuns
Copy link
Contributor Author

ankuns commented Apr 30, 2025

It seems we are stuck in a clinch, @bpienk @carlescufi , mediation needed.

@ankuns
Copy link
Contributor Author

ankuns commented May 6, 2025

Replaced by upstream modification approach and with [nrf fromlist] cherry-picked commit nrfconnect/sdk-zephyr#2833

@ankuns ankuns closed this May 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. doc-required PR must not be merged without tech writer approval.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants